Skip to content

fix(mock): stop leaking sibling variants into allOf-inherited mocks - #3431

Merged
melloware merged 2 commits into
orval-labs:masterfrom
wadakatu:fix/2155-discriminator-allof-sibling-leakage
May 24, 2026
Merged

fix(mock): stop leaking sibling variants into allOf-inherited mocks#3431
melloware merged 2 commits into
orval-labs:masterfrom
wadakatu:fix/2155-discriminator-allof-sibling-leakage

Conversation

@wadakatu

@wadakatu wadakatu commented May 24, 2026

Copy link
Copy Markdown
Contributor

Closes #2155

Closes the second half of #2155, the part PR #3429 deliberately left out. When a derived schema is shaped as

Item1:
  allOf:
    - $ref: '#/components/schemas/DiscriminatorParent'
    - type: object
      properties:
        property1: { type: string }

and the parent declares both discriminator: { mapping } and oneOf, resolveMockValue was re-expanding the parent's oneOf while building Item1's allOf-chain mock body. That inlined sibling factory calls into the derived variant's mock, so the produced object had shapes belonging to other variants:

// Before — Item1's mock contained Item2/Item3 calls and Item3 had a `{ undefined }` artifact:
export const getGetTestResponseItem3Mock = (overrideResponse = {}) => ({
  ...{...{...{ undefined },                                  // ← from oneOf collapsing
                                                              //   to undefined inside the allOf chain
    ...{ property3: faker.helpers.arrayElement([...]) },
  }, type: faker.helpers.arrayElement(['item3'] as const)},
  ...overrideResponse,
});
export const getGetTestResponseItem2Mock = (overrideResponse = {}) => ({
  ...{...{...faker.helpers.arrayElement([{ ...getGetTestResponseItem3Mock() }]),
  ...},
  ...overrideResponse,
});
export const getGetTestResponseItem1Mock = (overrideResponse = {}) => ({
  ...{...{...faker.helpers.arrayElement([
    { ...getGetTestResponseItem2Mock() },                    // ← Item1 carries Item2's shape
    { ...getGetTestResponseItem3Mock() },                    //   and Item3's shape
  ]), ...},
  ...overrideResponse,
});

// After — each variant only describes its own properties + constrained discriminator:
export const getGetTestResponseItem1Mock = (overrideResponse = {}) => ({
  ...{...{...{ property1: faker.helpers.arrayElement([...]) }},
    type: faker.helpers.arrayElement(['item1'] as const),
  },
  ...overrideResponse,
});
// (Item2/Item3 follow the same clean shape.)

Fix

In resolveMockValue, after loading a $ref whose schema has both discriminator and oneOf, check whether the resolution stack is currently inside one of that parent's mapping targets. If so, we are by construction expanding a specific variant of the union — the parent's oneOf is descriptive, not additive — so:

  1. Drop oneOf from a local copy of the parent before passing to getMockScalar (prevents sibling-factory inlining).
  2. Symmetrically with fix(mock): preserve discriminator value when oneOf parent declares the same property #3429's oneOf-side fix, also drop the discriminator key from the parent's properties (and required). Each variant already carries a constrained discriminator value via resolveDiscriminators, so the parent's free-choice enum would otherwise become dead code shadowed by the variant's constrained value through spread merge.
if (
  combine?.separator === 'allOf' &&
  newSchema.discriminator &&
  newSchema.oneOf
) {
  const mappingTargetNames = Object.values(parentDiscriminator.mapping ?? {})
    .map(ref => pascal(ref.split('/').pop() ?? ''));
  const expandingAsVariant = existingReferencedProperties
    .some(refName => mappingTargetNames.includes(refName));

  if (expandingAsVariant) {
    delete (newSchema as Record<string, unknown>).oneOf;
    // ...also strip propertyName from properties + required
  }
}

Why the mapping-target guard matters

The naive form ("schema has discriminator + oneOf and we're inside allOf → strip oneOf") breaks cases where a discriminator parent is referenced via allOf but the surrounding schema is not a variant. Concrete example from this repo's tests/specifications/one-of-nested.yaml:

Example2:
  type: object
  properties:
    kind: { type: string, enum: [example2] }
    expiry:
      allOf:
        - $ref: '#/components/schemas/PointInFuture'   # PointInFuture is the discriminator parent

Example2.expiry is a field holding a PointInFuture value. The mock for expiry must still randomize across PointInFutureAbsolute / PointInFutureRelative. Without the mapping-target check, the naive fix would emit an empty expiry. The check fixes this — Example2 is not in PointInFuture.discriminator.mapping, so the strip does not apply and the snapshot for that fixture is unchanged.

Tests

  • New fixture: tests/specifications/discriminator-oneof-allof.yaml (the issue's exact reproduction shape).
  • New mock config + snapshot under tests/__snapshots__/mock/discriminator-oneof-allof/.
  • Focused regression in tests/api-generation.spec.ts that asserts each per-variant factory body does not reference its siblings' factories — a more precise signal than the full-file snapshot diff alone.
  • Existing discriminator fixtures (discriminator-oneof-union from fix(mock): preserve discriminator value when oneOf parent declares the same property #3429, recursive-discriminator-allof, one-of-nested, polymorphic, boolean-discriminator, lowercase-discriminator) generate identically — verified locally with no snapshot diffs.

Type-generation circularity (orthogonal)

The model files generated from this fixture trip a separate, pre-existing core type-generation circularity:

generated/mock/discriminator-oneof-allof/model/item1.ts(10,13):
  error TS2456: Type alias 'Item1' circularly references itself.

Item1 is emitted as Omit<DiscriminatorTest, 'type'> & { type: Item1Type, property1?: string } while DiscriminatorTest = Item1 | Item2 | Item3, which is a TS type-alias cycle. This is unrelated to the mock generator — the same fixture exhibits the issue with or without this PR — and tackling it cleanly belongs in @orval/core's getCombineSchema / discriminator paths.

To keep the mock regression testable today, the new fixture is added to the tests/scripts/typecheck-generated.mjs exclusion list (alongside the existing MCP exclusion). Follow-up issue filed for the core type-circularity so it's not lost: #3432.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed mock generation for discriminated union types with allOf-inherited variants to avoid duplicated union/discriminator behavior in generated responses.
  • Tests

    • Added regression test ensuring inherited-variant mocks do not reference sibling factories.
    • Added generated mock snapshots and model types for the discriminator/oneOf/allOf scenario.
  • Chores

    • Added OpenAPI test spec, Orval mock config entry, and updated typecheck script exclusions.

Review Change Stack

Closes the second half of orval-labs#2155. When a derived schema is shaped as

  Item1: { allOf: [{ $ref: '#/components/schemas/DiscriminatorParent' }, ...] }

and the parent declares both `discriminator: { mapping }` and `oneOf`,
`resolveMockValue` was re-expanding the parent's `oneOf` while building the
allOf-chain mock body. That inlined sibling factory calls (e.g.
`getResponseItem2Mock()` and `getResponseItem3Mock()` inside `Item1`'s mock
body), so each derived variant's mock contained shapes belonging to its
siblings.

When a discriminator parent is being expanded under an allOf chain rooted at
one of its mapping targets, the current schema is — by construction — a
specific variant. The parent's `oneOf` is descriptive of the union, not
additive to this variant. Detect that situation (parent has both
`discriminator` and `oneOf`, current `combine?.separator === 'allOf'`, and at
least one mapping-target name is on the resolution stack via
`existingReferencedProperties`) and drop the `oneOf` side from a local copy of
the loaded parent schema before passing it to `getMockScalar`.

The mapping-target check is what keeps unrelated allOf wrappings safe: e.g.
`Example2.expiry: { allOf: [{ $ref: PointInFuture }] }` in
`tests/specifications/one-of-nested.yaml` is NOT a variant of `PointInFuture`,
just a field whose value is one of its variants, so its mock must keep
randomizing across `oneOf`.

Symmetrically with orval-labs#3429's oneOf-side fix, also drop the discriminator key
from the parent's `properties` (and the matching entry from `required`) in
the variant case. Each variant already encodes a constrained discriminator
value via `resolveDiscriminators`, so leaving the parent's free-choice enum
would just emit dead code immediately shadowed by the variant's constrained
value through spread merge.

### Tests

- `tests/specifications/discriminator-oneof-allof.yaml` mirrors the issue's
  exact reproduction (parent oneOf + discriminator + 3 `allOf`-inheriting
  variants).
- Focused regression test in `tests/api-generation.spec.ts` asserts each
  per-variant factory body does not reference its siblings' factories.
- Generated model files for this fixture trigger a separate, pre-existing
  core type-generation circularity (`DiscriminatorTest` -> `ItemN` via
  `Omit<...>`) that is orthogonal to the mock bug being fixed. The fixture is
  added to the `tests/scripts/typecheck-generated.mjs` exclusion list so the
  mock regression remains testable today; the underlying core type
  circularity can be tackled separately.

Closes orval-labs#2155
Copilot AI review requested due to automatic review settings May 24, 2026 14:57
@coderabbitai

coderabbitai Bot commented May 24, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 18c9951f-d392-4a65-97ff-3a0a4c784cf6

📥 Commits

Reviewing files that changed from the base of the PR and between b6fd6cf and db234a4.

📒 Files selected for processing (2)
  • tests/api-generation.spec.ts
  • tests/scripts/typecheck-generated.mjs

📝 Walkthrough

Walkthrough

This PR fixes mock generation for discriminator-based unions with oneOf and allOf composition. The core resolver logic now conditionally removes the discriminator parent's oneOf and discriminator property when expanding as a variant. Test specification, generated model types, endpoint mocks, and a regression test validate the fix.

Changes

Discriminator oneOf/allOf variant mock resolution

Layer / File(s) Summary
Core discriminator/oneOf/allOf resolution logic
packages/mock/src/faker/resolvers/value.ts
resolveMockValue detects variant expansion by comparing existingReferencedProperties against discriminator mapping targets. When expanding as a variant, it removes the discriminator parent's oneOf and deletes the discriminator property from properties and required (or deletes those fields entirely if empty) to prevent shadowed union behavior.
Test specification and generator configuration
tests/specifications/discriminator-oneof-allof.yaml, tests/configs/mock.config.ts, tests/scripts/typecheck-generated.mjs
OpenAPI 3.0.2 spec defines DiscriminatorTest with discriminator on type and oneOf/allOf-inherited variants (Item1, Item2, Item3); Orval config entry is added to generate mocks; typecheck excludes the fixture to cover the regression.
Generated model types and discriminator schemas
tests/__snapshots__/mock/discriminator-oneof-allof/model/*
Exports DiscriminatorTest union type, DiscriminatorTestType discriminator literal, and three variant types (Item1, Item2, Item3) each with their corresponding discriminator type (Item1Type, Item2Type, Item3Type).
Generated endpoint client and mock factories
tests/__snapshots__/mock/discriminator-oneof-allof/endpoints.ts
Exports axios getTest client helper, three variant mock object factories (getGetTestResponseItem1Mock, getGetTestResponseItem2Mock, getGetTestResponseItem3Mock), a discriminator union mock factory (getGetTestResponseMock), and MSW handler integration (getGetTestMockHandler, getDiscriminatorWithOneOfUnionAndAllOfInheritedVariantsMock).
Regression test validation
tests/api-generation.spec.ts
Vitest test verifies that allOf-inherited variant mock factories do not inline sibling variant factory references, confirming the resolver fix prevents duplicate union behavior in generated mocks.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • orval-labs/orval#3084: Also modifies packages/mock/src/faker/resolvers/value.ts's discriminator/oneOf handling at the resolver level.
  • orval-labs/orval#3426: Related changes in resolveMockValue for referenced-schema expansion and discriminator handling.
  • orval-labs/orval#3429: Similar fix that removes parent discriminator keys before deriving variants to avoid cross-variant inlining.

Suggested labels

mock

Suggested reviewers

  • melloware

Poem

🐰 A discriminator's tale in code and art,
oneOf and allOf once played their part.
The resolver hops in, tidy and kind,
removes the duplicate, leaves variants aligned.
Now mocks dance true — no sibling left to find. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main fix: preventing sibling variant factories from being inlined into allOf-inherited mock variants, which is the core issue addressed in the PR.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds a new OpenAPI fixture to reproduce a mock-generation regression involving discriminator oneOf unions with allOf-inherited variants, and updates the mock resolver + tests to prevent sibling factory leakage in generated variant mocks.

Changes:

  • Introduces discriminator-oneof-allof specification + mock generation config and snapshots.
  • Adds a focused regression test ensuring per-variant mock bodies don’t reference sibling factories.
  • Updates mock value resolution to drop parent oneOf (and discriminator property) when expanding a discriminator parent inside an allOf variant chain.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/specifications/discriminator-oneof-allof.yaml New fixture spec capturing the discriminator/oneOf + allOf-inheritance pattern.
tests/scripts/typecheck-generated.mjs Excludes the new fixture’s generated mock output from typechecking due to known circular types.
tests/configs/mock.config.ts Adds mock generation target for the new fixture.
tests/api-generation.spec.ts Adds regression test asserting variant mocks don’t contain sibling factory calls.
tests/snapshots/mock/discriminator-oneof-allof/model/item3Type.ts Snapshot for generated discriminator subtype constant/type.
tests/snapshots/mock/discriminator-oneof-allof/model/item3.ts Snapshot for generated Item3 model type.
tests/snapshots/mock/discriminator-oneof-allof/model/item2Type.ts Snapshot for generated discriminator subtype constant/type.
tests/snapshots/mock/discriminator-oneof-allof/model/item2.ts Snapshot for generated Item2 model type.
tests/snapshots/mock/discriminator-oneof-allof/model/item1Type.ts Snapshot for generated discriminator subtype constant/type.
tests/snapshots/mock/discriminator-oneof-allof/model/item1.ts Snapshot for generated Item1 model type.
tests/snapshots/mock/discriminator-oneof-allof/model/index.ts Snapshot barrel exports for generated model package.
tests/snapshots/mock/discriminator-oneof-allof/model/discriminatorTestType.ts Snapshot for generated discriminator enum-like constant/type.
tests/snapshots/mock/discriminator-oneof-allof/model/discriminatorTest.ts Snapshot for generated union model type.
tests/snapshots/mock/discriminator-oneof-allof/endpoints.ts Snapshot for generated endpoints + mock factories/handlers.
packages/mock/src/faker/resolvers/value.ts Fix: avoid re-expanding parent oneOf under allOf-variant expansion (prevents sibling factory leakage).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +60 to +62
if (folder === 'mock') {
config.exclude = ['generated/mock/discriminator-oneof-allof/**'];
}
Comment thread tests/api-generation.spec.ts Outdated
Comment on lines +541 to +548
const block = endpoints.slice(
endpoints.indexOf(`export const ${funcName}`),
endpoints.indexOf(
'export const ',
endpoints.indexOf(`export const ${funcName}`) + 1,
),
);
expect(

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/scripts/typecheck-generated.mjs (1)

52-62: ⚡ Quick win

Add an explicit tracking issue reference for this temporary exclusion.

The rationale is clear, but please include a concrete issue link/ID in this block so removal can be tracked and verified later.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/scripts/typecheck-generated.mjs` around lines 52 - 62, Add a comment
with a tracking issue reference inside the conditional that excludes the fixture
so removal can be tracked later: update the if (folder === 'mock') {
config.exclude = ['generated/mock/discriminator-oneof-allof/**']; } block by
appending a short comment that includes the issue link or ID (e.g., “tracking:
ISSUE-1234” or a GitHub issue URL) and a brief note that the exclusion is
temporary; ensure the comment is adjacent to the config.exclude entry so future
readers scanning the folder/config.exclude logic see the tracking reference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/api-generation.spec.ts`:
- Around line 540-547: The block extraction using endpoints.indexOf(`export
const ${funcName}`) can return -1 and produce an empty/truncated slice; update
the loop over variantBlocks to compute startIndex and nextIndex via
endpoints.indexOf, verify startIndex !== -1 and nextIndex !== -1 and nextIndex >
startIndex (or set nextIndex = endpoints.length when no subsequent 'export const
' exists) before slicing, and if the indices are invalid either fail the test or
skip the case with an explicit assertion error; reference the variables
variantBlocks, funcName, endpoints and the string lookup 'export const
${funcName}' when making the checks.

---

Nitpick comments:
In `@tests/scripts/typecheck-generated.mjs`:
- Around line 52-62: Add a comment with a tracking issue reference inside the
conditional that excludes the fixture so removal can be tracked later: update
the if (folder === 'mock') { config.exclude =
['generated/mock/discriminator-oneof-allof/**']; } block by appending a short
comment that includes the issue link or ID (e.g., “tracking: ISSUE-1234” or a
GitHub issue URL) and a brief note that the exclusion is temporary; ensure the
comment is adjacent to the config.exclude entry so future readers scanning the
folder/config.exclude logic see the tracking reference.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e05f960c-baaa-4b09-93fc-b96100157795

📥 Commits

Reviewing files that changed from the base of the PR and between f2b15fe and b6fd6cf.

📒 Files selected for processing (15)
  • packages/mock/src/faker/resolvers/value.ts
  • tests/__snapshots__/mock/discriminator-oneof-allof/endpoints.ts
  • tests/__snapshots__/mock/discriminator-oneof-allof/model/discriminatorTest.ts
  • tests/__snapshots__/mock/discriminator-oneof-allof/model/discriminatorTestType.ts
  • tests/__snapshots__/mock/discriminator-oneof-allof/model/index.ts
  • tests/__snapshots__/mock/discriminator-oneof-allof/model/item1.ts
  • tests/__snapshots__/mock/discriminator-oneof-allof/model/item1Type.ts
  • tests/__snapshots__/mock/discriminator-oneof-allof/model/item2.ts
  • tests/__snapshots__/mock/discriminator-oneof-allof/model/item2Type.ts
  • tests/__snapshots__/mock/discriminator-oneof-allof/model/item3.ts
  • tests/__snapshots__/mock/discriminator-oneof-allof/model/item3Type.ts
  • tests/api-generation.spec.ts
  • tests/configs/mock.config.ts
  • tests/scripts/typecheck-generated.mjs
  • tests/specifications/discriminator-oneof-allof.yaml

Comment thread tests/api-generation.spec.ts Outdated
- `tests/scripts/typecheck-generated.mjs`: build the `exclude` array
  incrementally so adding a future per-folder rule cannot silently overwrite
  another folder's exclusion. Functionally identical today since `mcp` and
  `mock` are mutually exclusive, but matches Copilot's defensive suggestion.
- `tests/api-generation.spec.ts` regression for orval-labs#2155: assert the block-start
  index is found before slicing, and fall back to `endpoints.length` when no
  trailing `export const` is present, so a missing/renamed factory fails the
  test deterministically instead of passing on an empty slice (Copilot /
  CodeRabbit review).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Discriminator propertyName is randomized twice in mocks causing a missmatch

3 participants